Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#2211416) Follow-up fixes for pstore #394

Merged

Conversation

dtardon
Copy link
Member

@dtardon dtardon commented May 31, 2023

Resolves: #2211416

Follow-up-for: #353

@mergify mergify bot added the pr/needs-ci Formerly needs-ci label May 31, 2023
@github-actions
Copy link

github-actions bot commented May 31, 2023

Tracker - 2211416

The following commits meet all requirements

commit upstream
21e8e38 - meson: remove libdw dependency from pstore systemd/systemd@5361f62
ba48981 - pstore: introduce tmpfiles.d/systemd-pstore.conf systemd/systemd@f00c366
7b80bc5 - tmpfiles: don't complain if we can't enable pstore in containers systemd/systemd@203c07c
7572e73 - pstore: don't enable crash_kexec_post_notifiers by default systemd/systemd@edb8c98

@systemd-rhel-bot systemd-rhel-bot added the pr/needs-review Formerly needs-review label May 31, 2023
@systemd-rhel-bot systemd-rhel-bot changed the title Follow-up pstore changes (#2211416) Follow-up pstore changes May 31, 2023
@systemd-rhel-bot systemd-rhel-bot added the tracker/unapproved Formerly needs-acks label May 31, 2023
@dtardon dtardon changed the title (#2211416) Follow-up pstore changes Follow-up fixes for pstore May 31, 2023
@systemd-rhel-bot systemd-rhel-bot changed the title Follow-up fixes for pstore (#2211416) Follow-up fixes for pstore May 31, 2023
@mergify mergify bot removed the pr/needs-ci Formerly needs-ci label May 31, 2023
msekletar
msekletar previously approved these changes Jun 14, 2023
Copy link
Member

@msekletar msekletar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@systemd-rhel-bot systemd-rhel-bot removed the pr/needs-review Formerly needs-review label Jun 14, 2023
@systemd-rhel-bot systemd-rhel-bot removed the tracker/unapproved Formerly needs-acks label Jul 13, 2023
@jamacku
Copy link
Member

jamacku commented Jul 17, 2023

@dtardon Please rebase this one. Thank you.

bluca and others added 4 commits July 17, 2023 16:12
systemd-pstore does not use any symbol from libdw, and never did,
but the dependency was listed since the beginning

(cherry picked from commit 5361f62)

Related: #2211416
The systemd pstore service archives the contents of /sys/fs/pstore
upon boot so that there is room for a subsequent dump.  The issue is
that while the service is present, the kernel still needs to be
configured to write data into the pstore. The kernel has two
parameters, crash_kexec_post_notifiers and printk.always_kmsg_dump,
that control writes into pstore.

The crash_kexec_post_notifiers parameter enables the kernel to write
dmesg (including stack trace) into pstore upon a panic, and
printk.always_kmsg_dump parameter enables the kernel to write dmesg
upon a shutdown (shutdown, reboot, halt).

As it stands today, these parameters are not managed/manipulated by
the systemd pstore service, and are solely reliant upon the user [to
have the foresight] to set them on the kernel command line at boot, or
post boot via sysfs. Furthermore, the user would need to set these
parameters in a persistent fashion so that that they are enabled on
subsequent reboots.

This patch introduces the setting of these two kernel parameters via
the systemd tmpfiles technique.

(cherry picked from commit f00c366)

Resolves: #2211416
commit f00c366 enabled
crash_kexec_post_notifiers by default regardless of whether pstore
is enabled or not.

The original intention to enabled this option by default is that
it only affects kernel post-panic behavior, so should have no harm.
But this is not true if the user wants a reliable kdump.

crash_kexec_post_notifiers is known to cause problem with kdump,
and it's documented in kernel. It's not easy to fix the problem
because of how kdump works. Kdump expects the crashed kernel to
jump to an pre-loaded crash kernel, so doing any extra job before
the jump will increase the risk.

It depends on the user to choose between having a reliable kdump or
some other post-panic debug mechanic.

So it's better to keep this config untouched by default, or it may put
kdump at higher risk of failing silently. User should enable it by
uncommenting the config line manually if pstore is always needed.

Also add a inline comment inform user about the potential issue.

Thanks to Dave Young for finding out this issue.

Fixes #16661

Signed-off-by: Kairui Song <kasong@redhat.com>
(cherry picked from commit edb8c98)

Related: #2211416
@mergify mergify bot added the pr/needs-ci Formerly needs-ci label Jul 17, 2023
@systemd-rhel-bot systemd-rhel-bot added the pr/needs-review Formerly needs-review label Jul 17, 2023
@mergify mergify bot removed the pr/needs-ci Formerly needs-ci label Jul 17, 2023
Copy link
Member

@msekletar msekletar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@systemd-rhel-bot systemd-rhel-bot removed the pr/needs-review Formerly needs-review label Aug 22, 2023
@systemd-rhel-bot systemd-rhel-bot merged commit 21e0afb into redhat-plumbers:main Aug 22, 2023
@dtardon dtardon deleted the bz2158832-pstore-followup branch August 23, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants